Skip to content

Allow Iceberg writes to use either append or fastAppend#15094

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
SergeySereda0218:allow_iceberg_writes_to_use_either_append_or_fastappend
Feb 1, 2023
Merged

Allow Iceberg writes to use either append or fastAppend#15094
ebyhr merged 1 commit intotrinodb:masterfrom
SergeySereda0218:allow_iceberg_writes_to_use_either_append_or_fastappend

Conversation

@SergeySereda0218
Copy link
Copy Markdown
Contributor

Description

Closes #14822
Allow Iceberg writes to use either append or fastAppend. Users in certain use cases may want to do compaction of Manifests async from writes. We should allow configuring fastAppend for that situation on a session level basis.

Additional context and related issues

New IcebergSessionProperty iceberg.merge_manifests_on_write . Default value - "true".
SET SESSION iceberg.merge_manifests_on_write = false to force fastAppend()
SET SESSION iceberg.merge_manifests_on_write = true to force append()

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

Iceberg

  • Allow to do compaction of Manifests asynchronously from writes by configuring property iceberg.merge_manifests_on_write on a session level. ({issue}14822)
# Section

Test_trino_cli
Test_IcebergQueryRunner

Copy link
Copy Markdown
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. A few minor comments, otherwise looks good.

Comment thread plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated
Comment thread plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated
Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash commits into one.

Comment thread plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated
Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for picking this up

@SergeySereda0218 SergeySereda0218 force-pushed the allow_iceberg_writes_to_use_either_append_or_fastappend branch 6 times, most recently from 63623f0 to 3f5f096 Compare November 26, 2022 20:35
@SergeySereda0218 SergeySereda0218 force-pushed the allow_iceberg_writes_to_use_either_append_or_fastappend branch from 3f5f096 to 995dc42 Compare November 27, 2022 10:41
@SergeySereda0218
Copy link
Copy Markdown
Contributor Author

Hi.
All code reviews are resolved and all tests are successfully passed.
Is there something else I should do in this PR for merge?

@alexjo2144 alexjo2144 requested a review from ebyhr December 13, 2022 14:39
@ebyhr ebyhr merged commit 5a34f27 into trinodb:master Feb 1, 2023
@github-actions github-actions Bot added this to the 407 milestone Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Allow Iceberg writes to use either append or fastAppend

4 participants